Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change required capability for published_node_ids in GetVolume #487

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Change required capability for published_node_ids in GetVolume #487

merged 1 commit into from
Sep 15, 2021

Conversation

bswartz
Copy link
Contributor

@bswartz bswartz commented Jul 21, 2021 •
edited
Loading

Instead of PUBLISH_UNPUBLISH_VOLUME, use the LIST_VOLUMES_PUBLISHED_NODES
capability to control the published_node_ids field in ControllerGetVolume.
This change is to be more consistent with the existing behavior. It is
a backwards-incompatible change, but the API is alpha.

Also clarify the LIST_VOLUMES_PUBLISHED_NODES docs.

Fixes: #486

@bswartz
Copy link
Contributor Author

bswartz commented Jul 21, 2021

@saad-ali @xing-yang please take a look at this

@xing-yang
Copy link
Contributor

LGTM

@jdef
Copy link
Member

jdef commented Jul 23, 2021

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

@bswartz
Copy link
Contributor Author

bswartz commented Jul 23, 2021

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

I agree with both these.

@bswartz
Change required capability for published_node_ids in GetVolume
d64255d 
Instead of PUBLISH_UNPUBLISH_VOLUME, use the LIST_VOLUMES_PUBLISHED_NODES
capability to control the published_node_ids field in ControllerGetVolume.
This change is to be more consistent with the existing behavior. It is
a backwards-incompatible change, but the API is alpha.

Also clarify the LIST_VOLUMES_PUBLISHED_NODES docs.
@bswartz bswartz force-pushed the get-volume-published-nodes-fix branch from 3d692ba to d64255d Compare July 23, 2021 18:11
@bswartz
Copy link
Contributor Author

bswartz commented Jul 23, 2021

@jdef Updated, PTAL.

@bswartz
Copy link
Contributor Author

bswartz commented Jul 28, 2021

This is ready to merge IMO

@saad-ali saad-ali mentioned this pull request Sep 15, 2021
Open
@saad-ali
Copy link
Member

/lgtm
/approve

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

I agree with both these.

Opened #489 to track that.

@saad-ali saad-ali merged commit 5b0d454 into container-storage-interface:master Sep 15, 2021
@zxh326 zxh326 mentioned this pull request Nov 7, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdef jdef jdef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

published_node_ids has different requirements in ListVolumes vs ControlerGetVolume
4 participants
@bswartz @xing-yang @jdef @saad-ali